Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: improve error message for policy failures #35633

Closed
wants to merge 6 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Oct 13, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

fixes: #35600

this doesn't attempt to fix this outside of running node's test suite since we cannot know when a package.json is expected or not except through the policy itself. in theory any test relying on "type" should also use this helper... but that is basically everything so it is fine to omit I think.

@bmeck bmeck requested a review from jasnell October 13, 2020 19:24
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 13, 2020
@bmeck bmeck requested a review from Trott October 13, 2020 20:54
test/common/index.js Outdated Show resolved Hide resolved
test/common/index.js Outdated Show resolved Hide resolved
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I'm sure we will both be happy to have these errors reported like this in CI rather than having to log into a failing CI host and poke around the file system.

@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 15, 2020
@Trott
Copy link
Member

Trott commented Oct 15, 2020

Oh, sorry, but I forgot that this will also need an entry in test/common/README.md. It would go in the "Common module API" section. The entries are in alphabetical order. Maybe something like this?

### `requireNoPackageJSONAbove()`

Throws an `AssertionError` if a `package.json` file is in any ancestor
directory. Such files may interfere with proper test functionality.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to lib/internal/url.js look unrelated and accidental?

bmeck and others added 6 commits October 18, 2020 06:12
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 18, 2020

The changes to lib/internal/url.js look unrelated and accidental?

Since they were pretty clearly unrelated and accidental, I rebased and added a fixup commit to remove them.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2020
@nodejs-github-bot

This comment has been minimized.

@codecov-io
Copy link

Codecov Report

Merging #35633 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #35633   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files         220      220           
  Lines       73681    73681           
=======================================
+ Hits        71031    71034    +3     
+ Misses       2650     2647    -3     
Impacted Files Coverage Δ
lib/internal/util/inspect.js 96.14% <0.00%> (+0.09%) ⬆️
lib/_http_server.js 98.45% <0.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe33aa...e6be5c7. Read the comment docs.

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 8, 2020
@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2020

Landed in 5b95f01

aduh95 pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #35633
Fixes: #35600
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 closed this Nov 9, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #35633
Fixes: #35600
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #35633
Fixes: #35600
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #35633
Fixes: #35600
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #35633
Fixes: #35600
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott pushed a commit that referenced this pull request Apr 23, 2021
Policy tests can fail if a `package.json` exists in any of the parent
directories above the test. The existing checks are done for the
ancestors of the test directory but some tests execute from the tmpdir.

PR-URL: #38285
Refs: #38088
Refs: #35600
Refs: #35633
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 29, 2021
Policy tests can fail if a `package.json` exists in any of the parent
directories above the test. The existing checks are done for the
ancestors of the test directory but some tests execute from the tmpdir.

PR-URL: #38285
Refs: #38088
Refs: #35600
Refs: #35633
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
Policy tests can fail if a `package.json` exists in any of the parent
directories above the test. The existing checks are done for the
ancestors of the test directory but some tests execute from the tmpdir.

PR-URL: #38285
Refs: #38088
Refs: #35600
Refs: #35633
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
Policy tests can fail if a `package.json` exists in any of the parent
directories above the test. The existing checks are done for the
ancestors of the test directory but some tests execute from the tmpdir.

PR-URL: #38285
Refs: #38088
Refs: #35600
Refs: #35633
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
Policy tests can fail if a `package.json` exists in any of the parent
directories above the test. The existing checks are done for the
ancestors of the test directory but some tests execute from the tmpdir.

PR-URL: #38285
Refs: #38088
Refs: #35600
Refs: #35633
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bmeck bmeck deleted the policy-test-err-message branch February 3, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

policy tests fail if there is a package.json in parent directories
6 participants